ipni: drop ad-fetch tracking, return adCid in piece status instead - #1450
ipni: drop ad-fetch tracking, return adCid in piece status instead#1450beck-8 wants to merge 1 commit into
Conversation
FilOzzy
added
the
team/fs-wg
|
If you think it is inappropriate to throw out the Ad information and let the user inquire by himself. And accepting this interface will be stuck, so you can put this part of the check in curio (I can't accept this kind of thing very well, and this is a remote call. You can call the interface of curio frequently to make curio frequently want to initiate cid requests. To handle this part, you need to add more Add cache status). |
BigLep
left a comment
BigLep
left a comment
There was a problem hiding this comment.
Thanks @beck-8 . This is interesting. I like the thinking of getting Curio out of being the "middle-man" between a client (e.g., Dealbot) and an IPNI instance (e.g., cid.contact). dealbot can just query cid.contact directly to determine whether the advertisment has been ingested.
I'm trying to think of any downsides of taking Curio out of the loop. I don't understand failure cases enough to know, but is there a world where it would be beneficial for Curio to know that a client's cids haven't been indexed yet by cid.contact?
Other things I would imagine adding in the PR:
- In the return struct, lets include the ipniInstance that the adCid was advertised to. I don't want client's hardcoding cid.contact.
- I think it would be good if the return structure also accounted for potentially multiple ipniInstances.
- (I'm suggesting the above so that we don't need to worry about client updates. Updating SPs is easier I think that getting clients to update if we ever added a different/additional IPNI instance besides cid.contact.)
I defer to @LexLuthr on final decisions here.
| - `adCreatedAt`: Timestamp the advertisement was created (omitted if not yet created). | ||
| - `adCid`: This piece's advertisement CID, once created. Curio doesn't check whether an indexer finished processing it - callers can, e.g. `GET https://cid.contact/sync/status/ad/{adCid}`. | ||
| - `advertised`: Whether the provider has sent an HTTP announce covering this ad. | ||
| - `advertisedAt`: Approximate, not a fixed record: it's the last known successful announce time for the *provider*, not this ad specifically, so it can drift forward on later calls once the provider announces newer ads, and is lost on a Curio restart (the provider re-announces its current head once on startup to recover it). |
There was a problem hiding this comment.
it's the last known successful announce time for the provider, not this ad specifically,
Is that because we're no longer storing state in the database (memory only), or has this always been the case. I would have thought we could be more precise with ourselves on the first advertising chain we announced that includes this adCid. If we don't know that, then I think we should drop this field.
There was a problem hiding this comment.
It turns out that there are three layers of judgment:
- The first layer is determined by whether ipni comes to fetch, and we delete this logic here. This is fixed
- The second layer is inferred by time. This will be offset.
- The third floor is a pocket bottom, fixed time
So here we have to make a choice, either to retain the original logic or possibly delete everything? Only keep the state?
I saw that there was another alarm in the previous design. It is to judge the gap between head and cid. Curio will send an alarm 1 hour behind.
I reconsidered it. This part is not elegant. And it is our consensus which IPNI to use now. It does not allow mistakes or problems. This website has been hard-coded in many downstream places. If you want to replace it, you need everyone to replace it I will make changes when I get more input from you. |
|
HI @beck-8 - thanks for engaging. I'll let @LexLuthr make the calls here as he knows more of the specifics. In general, I want to make sure we have some flexibility in our system to not always assume cid.contact. If cid.contact has problems again we'll likely spin up our own IPNI instance (not the current filecoinpin.contact with bespoke logic but a storetheindex instance that has the same endpoints as cid.contact). It would be great if that could "just work" by having SPs update config to use it and not require server API code and client code for that to work... But if that is too much extra we can bite the bullet then. My general position is I want to be able to adapt quickly on the content routing front, and API flexibility is one area where I think we can. |
3 participants
#1426 (comment)
His follow-up update. Replace #1426 @BigLep @LexLuthr @jennijuju
I chose to add the return adCid, so that the caller can choose his own check logic.
Users should verify the advertised information, but should not rely on the advertisedAt date. After confirming the advertised details, users should continue to check the ad's status and the cid.
Summary of Changes
ipni-provider.go: Droppedipni_ad_fetcheswrites; fixed a bug whereLastPublishTimenever got set after a restart;PublishInterval5s → 1s.pdp/handlers.go: Removedretrieved/syncedfields, addedadCidso callers can check sync status against the indexer themselves;advertised/advertisedAtlogic unchanged.pdp/README.md: Updated docs for the new response fields.ipni_ad_fetchestable, with matching downgrade.